-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(cloudflare): Add instrumentWorkflowWithSentry to instrument workflows
#16672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| return wrapRequestHandler({ options, request: context.request, context }, () => context.next()); | ||
| return wrapRequestHandler({ options, request: context.request, context: { ...context, props: {} } }, () => | ||
| context.next(), | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was due to updating to the latest Cloudflare types.
ExecutionContext now has a props property which isn't on EventPluginContext.
| { | ||
| options: opts, | ||
| request: event.request, | ||
| request: event.request as Request<unknown, IncomingRequestCfProperties<unknown>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type updates also broke this...
❌ Unsupported file format
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work - just some small comments.
Co-authored-by: Abhijeet Prasad <[email protected]>
…try/sentry-javascript into timfish/feat/cloudflare-workflows
It was tricky to instrument Cloudflare workflows! The code for a workflow might look simple but there is a lot of clever shenanigans going on under the hood in the runtime to allow suspension of workflows. Workflows can be hibernated at any time and all state/context inside the your workflow class and elsewhere is lost. Ideally we want all of our step runs to have the same
trace_idso all steps in a workflow run are linked together and all steps should have the same sampling decision.To work around the state limitations, we use the workflow
instanceIdas both the Sentrytrace_idand the last 4 characters are used to generate thesample_randused in the sampling decision. Cloudflare uses uuid's by default forinstanceIdbut users do have the option of passing their own IDs. If users are supplying their owninstanceId's, they need to be both random and a 32 character uuid (with or without hyphens) or the Sentry instrumentation will throw an error.Points worthy of note:
enableDedupeconfig option (docs hidden) which removes thededupeIntegrationfor workflows. We want to get duplicate errors for step retriesWorkflowStepobject in another class. The Cloudflare step object is native so it's properties can't be overridden or proxiedin_app: falsestep.sleep,step.sleepUntilorstep.waitForEventbecause code doesn't run after the Cloudflare native function returnssetPropagationContextdirectly on the isolation context didn't work. It needed anotherwithScopeinside forsetPropagationContextto work. @mydea is that expected?Here is an example trace showing the final step failing (throwing) 6 times before completing successfully. The exponential retry backoff is clearly visible.